POC of using cargo package for installation#68
POC of using cargo package for installation#68luca-della-vedova wants to merge 1 commit intocolcon:cottsay/install-libsfrom
Conversation
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Blast545
left a comment
There was a problem hiding this comment.
I didn't test the example, but checked the code and the documentation of cargo package and this LGTM
| # REVERT, remove test dependency for false positive circular dep error | ||
| 'build': depends | build_depends, |
There was a problem hiding this comment.
Do you know why this causes the circular dep problem?
There was a problem hiding this comment.
It's an interesting issue.
Cargo itself actually allows circular dependencies for dev dependencies but colcon doesn't, so if we add all dev-dependencies to our build depends, we might fall into some false positives.
Details on when and why is this allowed are in the cargo book. From my understanding it's because a test is a standalone target that is built separately from the library so there isn't strictly a circular dependency.
The documentation advises against this practice but, of course, a lot of packages in the wild go and use it, such as tokio or serde that are very widespread.
Note that however this is just a hack to be able to test this PR, I don't think we should merge this change. I believe what we would really want is indeed include dev dependencies in our dependency tree, but at the same time disregard dependency cycle errors only when they occur while resolving dev dependencies (and perhaps only cargo dev dependencies). This is however quite tricky and we should probably explore it separately.
|
Sorry for the long silence on this, but your thorough explanation on this has led me down quite a rabbit hole. In short: you're absolutely right. You've convinced me that "normalizing" the manifest is absolutely necessary when we install it into the workspace. Further, I now that believe it extends beyond "library" crates and applies to all participants in a workspace, even those that only build binaries. I have also discovered (I believe) that it isn't possible to patch a dependency that refers to a sibling a package's workspace. At first glance, it might seem okay to forego the patch and let cargo consume the dependency directly from the source directory, but that could result in unexpected behavior when a downstream build in a separate workspace tries to use that same dependency and consumes it from the I'm experimenting with code which unconditionally normalizes the manifest before operating on it. Maybe we can get away with only doing it for workspace packages, but until we can really enumerate everything that happens during cargo's manifest normalization process, I'm inclined to just do it every time. The only supported way to normalize the manifest, as far as I can tell, is using To use the manifest during our operations (namely Since we're essentially re-homing the whole crate at this point, the thought occurred to me that maybe we should just build it there and not use It's worth noting that making colcon-cargo/test/rust-sample-package/Cargo.toml Lines 9 to 10 in 518a4f9 My (current) conclusionColcon operates on sources on a per-package basis. Effort is made to isolate each package, sometimes to validate proper dependency declaration, and sometimes to ensure that the resulting install space can be used by downstream processes or "child" workspaces. In many ways this is in opposition to much of how the cargo tool consumes workspace members, so we should make an effort to "de-workspace" the packages using cargo's manifest normalization. This should (help) ensure that we aren't crossing the package boundary in cargo in opposition with colcon's efforts, and allows us to patch dependencies that are (or were previously) workspace siblings. Dragons
Sorry lots of text there. I'll try to follow up with some prototypes and examples, and share any new discoveries along the way. |
This PR targets #67 and illustrates the idea I have been toying with about using
cargo packageto install libraries throughcolcon-cargo.It's just a POC to facilitate discussion, ideally we can at least be smarter with caching / remove duplicated work and rework install folder structures.
The proposed implementation in #67 uses
cargo package --listto get a list of files that would be installed then uses the output to copy files from the package's source directory into the install space.This however, is not sufficient for packages that are in a cargo workspace, since
cargo packageactually performs a series of operations to theCargo.tomlto, for example, remove relative path dependencies, resolve workspace dependencies etc. (point 1 and some of point 2 here).This shows in the fact that what we actually install in #67 doesn't compile if applied to a more complex package.
I came up with a "simple" test case for both #67 and this PR to show the difference.
Setup
Create a workspace and bring a custom tokio (where I just added a public
hello_worldfunction) and a POC crate that tests it (has a library and an executable that call the new tokio function).mkdir -p tokio_ws/src cd tokio_ws/src git clone https://github.com/luca-della-vedova/tokio git clone https://github.com/luca-della-vedova/cargo_custom_tokio_packageTest with #67
Remove
dev_dependsfrom here as done in this PR to avoid circular dependency errorsSince the tokio Cargo.toml has one of these "workspace inheritances", the build will actually fail, making the crate effectively non functional:
The snippet is preserved when copying the file and cannot be resolved since the file is copied to a different location and its workspace's
Cargo.tomlis not in its parent folder anymore.Test with this branch
You will see that the package builds correctly, meaning the workspace migration was done by Cargo.
Inspecting the installed Cargo.toml, we can see that the linter configuration was migrated to a standalone configuration that does not reference the workspace's
Cargo.toml:Additionally, if now you build the sample package:
It will build successfully and running it will work, meaning the hardcoded patch correctly resolved to a tokio with the custom function:
Conclusion
cargo packagedoes a lot more for non-toy projects. I demonstrated this with tokio but most major projects out there have workspace dependencies that would make them non functional (i.e. serde).colcon-ros-cargo/ message generation. Patching is hardcoded but left as future work (perhaps through pallet patcher?)cargo packagealways builds a package even if it was unchanged, since I see that install times are quite significant even when no changes are made.